Replace FastAPI pure str comparison with SemVer comparison #433
Conversation
There was a problem hiding this comment.
Outside perspective: I like how this is implemented with no new dependencies. I might have been tempted to lean on semver3, but understand this might create Python compatibility and other downstream complications. Will be interested to see how the discussion progresses.
|
Checking in on any progress getting this merged? We are a little stuck on upgrades to fast-api while Rollbar misinterprets their 100 version as < 0.41+. |
There was a problem hiding this comment.
Hey @ayharano, thanks for the great work on this! Like @cforcey, I appreciate the fact that there are no 3rd party dependancies!
I added some unit tests based on the SemVer comparison on https://semver.org/#spec-item-11 with some additional versions to check FastAPI's non . separated pre-release identifiers (e.g. alpha2 not alpha.2).
Some of the tests are failing. I think it has to do with the - separator for the pre-release identifiers. I ran out of time today to dig into it. @ayharano is that something you can do?
rollbar/contrib/fastapi/utils.py
Outdated
| def is_current_version_higher_or_equal(current_version, min_version): | ||
| if current_version == min_version: | ||
| return True | ||
|
|
||
| for current_as_string, min_as_string in zip( | ||
| current_version.split('.'), | ||
| min_version.split('.'), | ||
| ): | ||
| if current_as_string == min_as_string: | ||
| continue | ||
|
|
||
| try: | ||
| current_as_int = int(current_as_string) | ||
| except ValueError: | ||
| current_as_int = None | ||
|
|
||
| try: | ||
| min_as_int = int(min_as_string) | ||
| except ValueError: | ||
| min_as_int = None | ||
|
|
||
| if current_as_int is None or min_as_int is None: | ||
| # If one of the parts fails the int conversion, compare as string | ||
| return current_as_string > min_as_string | ||
| else: | ||
| if current_as_int == min_as_int: | ||
| continue | ||
| return current_as_int > min_as_int | ||
|
|
||
| # Somehow the comparison didn't properly finish - defaulting to False | ||
| return False |
There was a problem hiding this comment.
Per PEP 440, this could probably be simplified.
FastAPI's version needs to be PEP440 compliant, irrespective of SemVer's spec.
Unfortunately that means fully-complaint checking requires things like epoch prefixes and -dev sorting before -beta.
Fortunately however, I think we can outright ignore everything except the release segment (i.e., ignore -alphaN, -betaN, -devN, preN, etc.) for the purposes of compatibility checking. That is, unless we want to say the min version of FastAPI supported by Rollbar is something like 0.104.0-beta2 and 0.104.0-beta1 is not allowed. That seems very unlikely to be useful.
| def is_current_version_higher_or_equal(current_version, min_version): | |
| if current_version == min_version: | |
| return True | |
| for current_as_string, min_as_string in zip( | |
| current_version.split('.'), | |
| min_version.split('.'), | |
| ): | |
| if current_as_string == min_as_string: | |
| continue | |
| try: | |
| current_as_int = int(current_as_string) | |
| except ValueError: | |
| current_as_int = None | |
| try: | |
| min_as_int = int(min_as_string) | |
| except ValueError: | |
| min_as_int = None | |
| if current_as_int is None or min_as_int is None: | |
| # If one of the parts fails the int conversion, compare as string | |
| return current_as_string > min_as_string | |
| else: | |
| if current_as_int == min_as_int: | |
| continue | |
| return current_as_int > min_as_int | |
| # Somehow the comparison didn't properly finish - defaulting to False | |
| return False | |
| def is_current_version_higher_or_equal(current_version, min_version): | |
| # We can ignore everything except the release segment | |
| current = tuple(map(int, re.findall(r"(?:^|[.])([0-9]+)", current_version))) | |
| minimum = tuple(map(int, re.findall(r"(?:^|[.])([0-9]+)", min_version))) | |
| return current >= minimum |
There was a problem hiding this comment.
@johnchristopherjones I agree that only trying to compare the release segment seems reasonable.
However, that regex also captures pre-release and dev segments if they include a .[0-9]. This does mean that 1.0.0-alpha.1 would become the tuple (1, 0, 0, 1). I am not great at regex. So, after a few failed attempts to solve this, I resorted to solving it in Python.
Description of the change
This is a solution proposal for #432: the current FastAPI version comparison fails for version
0.100.0-beta2because the lexicographic comparison of the string value fails because"1"is less than"4"even if the numerical value100is higher than41.Before:
After:
Type of change
Related issues
Checklists
Development
Code review